Skip to content

Conversation

@jostnes
Copy link
Contributor

@jostnes jostnes commented Dec 5, 2022

Description

This test was previously skipped as it was not implemented, this is the implementation of the test. This test tests that the user should be able to navigate through all the stats tabs and the stats will be updated to the correct timeframe.

Testing instructions

Test that test_load_stats_screen passes locally and in CI, also feel free to test failing scenarios (updating assertion should fail the test)


  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@jostnes jostnes added feature: stats Related to stats, including Top Performers. category: ui tests Related to UI testing. labels Dec 5, 2022
@jostnes jostnes requested a review from a team as a code owner December 5, 2022 10:21
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 5, 2022

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8310-1c0d0ec on your iPhone

If you need access to App Center, please ask a maintainer to add you.

Comment on lines 31 to 32
func tapTimeframeTab(timeframe: String) -> MyStoreScreen {
app.cells.staticTexts[timeframe].tap()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT of using the parent cell identifier instead? Example for This Month:

Screenshot 2022-12-05 at 15 15 28

This will make the test less dependent on the localization during tab selection (you will still have a human-readable interval mentioned in the assertion of the chart hint text).

Suggested change
func tapTimeframeTab(timeframe: String) -> MyStoreScreen {
app.cells.staticTexts[timeframe].tap()
func tapTimeframeTab(timeframeID: String) -> MyStoreScreen {
app.cells[timeframeID].tap()

The respective values for IDs to use in calling functions would be:

  • period-data-today-tab
  • period-data-thisWeek-tab
  • period-data-thisMonth-tab
  • period-data-thisYear-tab

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yup i agree with your point, this is updated in 27a1d7c

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing this @jostnes!

Comment on lines +49 to +54
func verifyStatsForTimeframeLoaded(timeframe: String) -> MyStoreScreen {
let textPredicate = NSPredicate(format: "label MATCHES %@", "Store revenue chart \(timeframe)")
XCTAssertTrue(app.images.containing(textPredicate).element.exists, "\(timeframe) chart not displayed")

return self
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT of extending this asserting function with one more check: making sure that the tab of interest is selected? Example of This Year tab being selected:

SCR-20221205-lqg

(For this you'll also need to provide the second argument with cell ID):

Suggested change
func verifyStatsForTimeframeLoaded(timeframe: String) -> MyStoreScreen {
let textPredicate = NSPredicate(format: "label MATCHES %@", "Store revenue chart \(timeframe)")
XCTAssertTrue(app.images.containing(textPredicate).element.exists, "\(timeframe) chart not displayed")
return self
}
func verifyStatsForTimeframeLoaded(timeframe: String, timeframeID: String) -> MyStoreScreen {
let textPredicate = NSPredicate(format: "label MATCHES %@", "Store revenue chart \(timeframe)")
XCTAssertTrue(app.images.containing(textPredicate).element.exists, "\(timeframe) chart not displayed")
XCTAssertTrue(app.cells[timeframeID].isSelected, "\(timeframeID) cell not selected")
return self
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm leaving this as is, for now. thinking of a scenario where the tab is not selected (e.g. if tap didn't work), the image wouldn't change too so i think that extra check isn't necessary for this test to work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally 👍 to leave it with no changes, just wanted to extend my thoughts about the reason for proposal: as you say, the test will still fail if tab selection did not work, because of incorrect chart name. My proposal was more about increasing the test coverage (e.g. checking for one field in the person details page VS checking for all fields). Also, it would cover a totally made up scenario, when tab switch animation did not work, but the screen itself is updated with correct chart and data. (just my thoughts behind it, let's leave it as it is now).

func skipTillImplemented(file: StaticString = #file, line: UInt = #line) throws {
try XCTSkipIf(true,
"Skipping until test is properly implemented", file: file, line: line)
.verifyTodayStatsLoaded()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might look redundant (and it most probably is), but I had an idea of also adding a function that selects the Today tab, and calling it before checking the Today stats.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm leaving this as is too, i think not adding that extra tap ensures that upon signing in the merchant would land on the expected tab (Today) instead of other tabs

Copy link
Contributor

@pachlava pachlava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jostnes !

Thanks for bringing this test is shape! It runs well locally for me, and on Buildkite as well 👍

I left three comments that would improve the test stability/coverage, IMO. Feel free to implement them or leave and :shipit: as is 🙂

@jostnes jostnes added this to the 11.6 milestone Dec 6, 2022
@jostnes jostnes merged commit 7af938d into trunk Dec 6, 2022
@jostnes jostnes deleted the add-load-stats-test branch December 6, 2022 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: ui tests Related to UI testing. feature: stats Related to stats, including Top Performers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants